Skip to content

Generate AST when importing a cpp file #4790

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Jan 14, 2025
Merged

Generate AST when importing a cpp file #4790

merged 14 commits into from
Jan 14, 2025

Conversation

@bricknerb
Copy link
Contributor

@bricknerb bricknerb commented Jan 10, 2025 •
edited
Loading

Ignore the AST and support a single Cpp import, for now.
Report cpp compilation errors and warnings.
Part of #4666

@bricknerb bricknerb requested a review from jonmeow January 10, 2025 15:40
@github-actions github-actions bot requested a review from danakj January 10, 2025 15:41
@bricknerb
Generate AST when importing cpp files
844ad0f 
Ignore the AST, for now.
Report cpp compilation errors and warnings.
Part of carbon-language#4666
danakj
danakj reviewed Jan 10, 2025
View reviewed changes
Copy link
Contributor

@danakj danakj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple suggestions and questions

jonmeow
jonmeow reviewed Jan 10, 2025
View reviewed changes
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

I'm looking at this, and thought of a lateral issue... We're fuzz testing, and as soon as the fuzzer discovers it can write C++ imports we'll end up fuzz testing Clang, and Clang is not crash-resilient. The driver already has a DriverEnv::fuzzing flag, can you have it lead to an error when set and importing from Cpp?

@bricknerb
Address review comments:
ff87420 
1. Change diagnostic ids.
2. Split file read error into a note.
3. Added bad import Cpp tests.
4. Remove `is_cpp` and move the condition higher.
@bricknerb
Explicitly unsupport multiple Cpp imports
941ce84
@bricknerb bricknerb changed the title Generate AST when importing cpp files Generate AST when importing a cpp file Jan 13, 2025
@bricknerb
Copy link
Contributor Author

bricknerb commented Jan 13, 2025

This is great!

I'm looking at this, and thought of a lateral issue... We're fuzz testing, and as soon as the fuzzer discovers it can write C++ imports we'll end up fuzz testing Clang, and Clang is not crash-resilient. The driver already has a DriverEnv::fuzzing flag, can you have it lead to an error when set and importing from Cpp?

I've added an error for that.

@bricknerb bricknerb requested review from danakj and jonmeow January 13, 2025 13:54
danakj
danakj reviewed Jan 13, 2025
View reviewed changes
jonmeow
jonmeow reviewed Jan 13, 2025
View reviewed changes
jonmeow
jonmeow reviewed Jan 13, 2025
View reviewed changes
@bricknerb bricknerb requested review from danakj and jonmeow January 14, 2025 10:32
jonmeow
jonmeow reviewed Jan 14, 2025
View reviewed changes
jonmeow
jonmeow approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

danakj
danakj reviewed Jan 14, 2025
View reviewed changes
@bricknerb bricknerb enabled auto-merge January 14, 2025 19:44
danakj
danakj reviewed Jan 14, 2025
View reviewed changes
@bricknerb bricknerb added this pull request to the merge queue Jan 14, 2025
Merged via the queue into carbon-language:trunk with commit 5b70a3e Jan 14, 2025
8 checks passed
@bricknerb bricknerb deleted the interop3 branch January 14, 2025 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonmeow jonmeow jonmeow approved these changes

@danakj danakj danakj approved these changes

Assignees

No one assigned

Labels

toolchain

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bricknerb @danakj @jonmeow